feat: geofence-based geographic targeting for programs (re-land from #76)#273
feat: geofence-based geographic targeting for programs (re-land from #76)#273gonzalesedwin1123 wants to merge 8 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces geofence-based geographic targeting for OpenSPP programs by adding the new spp_program_geofence module and extending spp_gis to support complex geometry types like MultiPolygon and GeometryCollection. It also replaces vocabulary-based geofence tags with a dedicated spp.gis.geofence.tag model, complete with migration scripts, and implements an OpenStreetMap fallback when no MapTiler API key is configured. The review feedback focuses on improving performance and code quality in the new Odoo models and JS widgets. Key recommendations include avoiding map re-creation on every patch in the GIS edit widget, optimizing recordset operations (using subtraction instead of lambda filtering and avoiding heavy NOT IN clauses in search domains), checking for active records in asynchronous tasks, and wrapping user-facing strings in _() for internationalization.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def _import_registrants(self, new_beneficiaries, state="draft", do_count=False): | ||
| _logger.info("spp_program_geofence: Importing %s beneficiaries", len(new_beneficiaries)) | ||
| self.env["spp.program.membership"].create( | ||
| [{"program_id": self.program_id.id, "partner_id": b.id, "state": state} for b in new_beneficiaries] | ||
| ) |
There was a problem hiding this comment.
In Odoo, when validating a Many2one relation in asynchronous or background tasks (such as _import_registrants which is queued via delayable), you should explicitly check if the referenced record is active (e.g., not self.program_id.active) in addition to checking its existence. A Many2one field still dereferences a deactivated record as a truthy recordset, which can bypass simple existence checks if the record is deactivated after the task is queued.
Additionally, iterating over new_beneficiaries.ids in the list comprehension is significantly more efficient than iterating over the new_beneficiaries recordset directly, as it avoids instantiating singleton record objects for each iteration.
| def _import_registrants(self, new_beneficiaries, state="draft", do_count=False): | |
| _logger.info("spp_program_geofence: Importing %s beneficiaries", len(new_beneficiaries)) | |
| self.env["spp.program.membership"].create( | |
| [{"program_id": self.program_id.id, "partner_id": b.id, "state": state} for b in new_beneficiaries] | |
| ) | |
| def _import_registrants(self, new_beneficiaries, state="draft", do_count=False): | |
| self.ensure_one() | |
| if not self.program_id or not self.program_id.active: | |
| _logger.warning("spp_program_geofence: Program is inactive or deleted, skipping import.") | |
| return | |
| _logger.info("spp_program_geofence: Importing %s beneficiaries", len(new_beneficiaries)) | |
| self.env["spp.program.membership"].create( | |
| [{"program_id": self.program_id.id, "partner_id": partner_id, "state": state} for partner_id in new_beneficiaries.ids] | |
| ) |
References
- In Odoo, when validating a Many2one relation in asynchronous or background tasks, explicitly check if the referenced record is active (e.g.,
not record.active) in addition to checking its existence. A Many2one field still dereferences a deactivated record as a truthy recordset, which can bypass simple existence checks if the record is deactivated after the task is queued.
| onPatched(() => { | ||
| this.defaultZoom = this.map.getZoom(); | ||
| this.renderMap(); | ||
| this.onLoadMap(); | ||
| this.addDrawInteraction(); | ||
| }); |
There was a problem hiding this comment.
Re-creating the entire MapLibre map instance on every patch/update is extremely inefficient. It causes map flickering, resets the map state (such as user zoom/pan), and can rapidly exhaust the browser's WebGL context limit, leading to context loss errors.
Instead, you should check if the geometry value has actually changed, and only re-render the map or update the draw features when a real change occurs. (Note: Remember to also initialize this._lastValue = this.props.record.data[this.props.name]; in onMounted to prevent unnecessary re-renders on the first patch).
onPatched(() => {
const currentValue = this.props.record.data[this.props.name];
if (currentValue !== this._lastValue) {
this._lastValue = currentValue;
this.defaultZoom = this.map.getZoom();
this.renderMap();
this.addDrawInteraction();
}
});| tier2_domain = base_domain + [ | ||
| ("area_id", "in", matching_areas.ids), | ||
| ("id", "not in", tier1.ids), | ||
| ] |
There was a problem hiding this comment.
Using ("id", "not in", tier1.ids) in the search domain is a performance anti-pattern in Odoo when tier1 contains a large number of records. This generates a heavy SQL query with a massive NOT IN clause.
Since the method returns tier1 | tier2 at the end, and Odoo's recordset union operator (|) automatically handles deduplication in Python extremely efficiently, you can safely remove the not in filter from the database query.
tier2_domain = base_domain + [
("area_id", "in", matching_areas.ids),
]| existing_partner_ids = set(self.program_id.get_beneficiaries().mapped("partner_id").ids) | ||
| new_beneficiaries = new_beneficiaries.filtered(lambda r: r.id not in existing_partner_ids) |
There was a problem hiding this comment.
Filtering a recordset using a Python lambda with not in on a set of IDs is less efficient and less idiomatic than using Odoo's built-in recordset subtraction operator (-).
You can simplify this to a single subtraction operation, which is faster and cleaner.
| existing_partner_ids = set(self.program_id.get_beneficiaries().mapped("partner_id").ids) | |
| new_beneficiaries = new_beneficiaries.filtered(lambda r: r.id not in existing_partner_ids) | |
| existing_partners = self.program_id.get_beneficiaries().mapped("partner_id") | |
| new_beneficiaries -= existing_partners |
| program.message_post(body=f"Import of {len(new_beneficiaries)} beneficiaries started.") | ||
| program.write({"is_locked": True, "locked_reason": "Importing beneficiaries"}) |
There was a problem hiding this comment.
These user-facing log messages and lock reasons are hardcoded strings and should be wrapped in _() to support internationalization (i18n).
| program.message_post(body=f"Import of {len(new_beneficiaries)} beneficiaries started.") | |
| program.write({"is_locked": True, "locked_reason": "Importing beneficiaries"}) | |
| program.message_post(body=_("Import of %s beneficiaries started.") % len(new_beneficiaries)) | |
| program.write({"is_locked": True, "locked_reason": _("Importing beneficiaries")}) |
| _logger.exception("Geofence eligibility preview failed for manager %s", self.id) | ||
| self.preview_count = 0 | ||
| self.preview_error = "Preview failed. Check the server logs for details." | ||
| return { | ||
| "type": "ir.actions.client", | ||
| "tag": "display_notification", | ||
| "params": { | ||
| "title": "Preview Complete", | ||
| "message": f"{self.preview_count} registrants match the current geofences.", | ||
| "sticky": False, | ||
| "type": "success" if not self.preview_error else "warning", |
There was a problem hiding this comment.
These user-facing notification titles, messages, and preview errors are hardcoded strings and should be wrapped in _() to support internationalization (i18n).
self.preview_error = _("Preview failed. Check the server logs for details.")
return {
"type": "ir.actions.client",
"tag": "display_notification",
"params": {
"title": _("Preview Complete"),
"message": _("%s registrants match the current geofences.") % self.preview_count,
"sticky": False,
"type": "success" if not self.preview_error else "warning",
},
}![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…tion (from #76) Re-lands the PR #76 description scope: spp_gis complex-geometry operators (MultiPolygon/GeometryCollection + distance buffering), OSM fallback when no MapTiler key is configured, renderer/edit-widget lifecycle fixes, geofence uuid in GeoJSON output, the spp.gis.geofence.tag model, and the new spp_program_geofence module (geofence eligibility manager, program UI, creation wizard). Adds what the original PR lacked: a migration pair remapping existing vocabulary-based geofence tag links (release v19.0.2.0.0 schema) onto spp.gis.geofence.tag records — the rel table is reused, so legacy rows are parked pre-upgrade and restored post-upgrade. Includes regression tests.
…generate spp_gis README Semgrep flagged f-string SQL in the tag migration scripts; identifiers were constants but composed SQL via psycopg2.sql removes the pattern entirely. README.rst regenerated from the updated HISTORY fragment (in-scope module only).
gonzalesedwin1123
force-pushed
the
reland/gis-geofence
branch
from
835401d to
9963347
Compare
Replaces psycopg2.sql identifier composition with prebuilt literal queries (the aux table name and the valid-link filter are compile-time constants), clearing the odoo-sql-injection-string-format code-scanning alerts. Values remain parameterized; behavior unchanged (spp_gis suite 92/0).
Every module carries a readme/HISTORY.md; the module was created in #76 without one. README.rst History section will be aligned to CI's renderer output in a follow-up commit.
…r-facing strings in _() - README.rst History section applied verbatim from CI's pre-commit diff. - Address gemini-code-assist review: translatable message_post/lock-reason and preview notification strings (i18n).
…back DESCRIPTION.md predates the geofence move (#74) and did not mention the capabilities this PR adds. README will be aligned to CI's renderer output in a follow-up commit.
|
Disposition of gemini-code-assist findings: Applied (commit 9d9afdb): i18n Deferred as follow-ups (pre-existing #76 code; this PR re-lands reviewed behavior and we're keeping the diff scoped):
|
… ladder The module's fragments use #/## headings; ### produced an RST title-level skip that crashed oca-gen-addon-readme.
…fence READMEs regenerated by CI's pinned generator from the updated DESCRIPTION and new HISTORY fragments; applied verbatim from its pre-commit diff.
Review & merge order for the PR #76 re-land stackThis PR is part of a 10-PR stack re-landing reverted #76 (revert: #271) in description-consistent pieces. Reviews within a batch can run in parallel; order only matters at merge time. All PR descriptions are self-contained. Batch 1 — review now, merge in any order (independent, CI green)
Batch 2 — merge strictly after prerequisites
Cautions
Verification summary per PR is in each description; the deferred gemini-code-assist suggestions are documented as comments on the affected PRs. |
2 participants
Re-lands the scope described in reverted PR #76 (revert: #271), as part of splitting that PR into description-consistent pieces. This description incorporates the relevant content of #76 so reviewers do not need to open it; everything listed below is contained in THIS PR's diff.
Summary
spp_program_geofencemodule for geofence-based program targeting and eligibility management.spp_gisspatial operators to supportMultiPolygonandGeometryCollection.YOUR_MAPTILER_API_KEY_HEREtreated as unconfigured).Functional detail (from #76, verified against this diff)
spp_program_geofence(new module)geofence_idsandgeofence_count.spp.program.membership.manager.geofence) with:groupsrestriction avoids exposing items to users lacking read access).spp_gisST_GeomFromGeoJSON; forMultiPolygon/GeometryCollection,(geojson, distance)appliesST_Buffer(...)correctly (EPSG:3857 transform parity when SRID is 4326), with regression tests for the distance-buffer path on both complex types.uuidas feature id; newspp.gis.geofence.tagmodel replaces vocabulary-based geofence tags (+ACLs).Added on top of #76 (not in the original)
spp_gis 19.0.2.1.0): geofencetag_idschanges co-model fromspp.vocabularytospp.gis.geofence.tagover the same rel table. Release v19.0.2.0.0 (Biliran) ships the old schema, so a pre-migration parks legacy links (the FK swap would otherwise fail) and a post-migration recreates them as tag records. Fully literal, value-parameterized SQL. Regression tests: remap, dedup across geofences, valid-link survival, fresh-DB no-op.spp_program_geofence: initialreadme/HISTORY.md; user-facing strings wrapped in_()for i18n (gemini-code-assist review).spp_gis:readme/DESCRIPTION.mdupdated to describe geofences, complex-geometry queries, and the OSM fallback; version bump + HISTORY entry; README rendered by CI's pinned generator.Not in this PR (other pieces of #76, re-landed separately)
spp_hazard CAP severity (#274), spp_cel_domain (#275), spp_api_v2 (#276), PHL demo data (#277), infra/tooling (#278, merged), spp_metric_service (#279), spp_gis_report (#280), spp_api_v2_gis (#281), spp_mis_demo_v2 (#282).
Verification
./spp t spp_gis: 92 passed, 0 failed (includes 4 migration tests)./spp t spp_program_geofence: 31 passed, 0 failedodoo-sql-injection-string-formatalerts #1837–#1842: fixed (literal SQL)